Fixed point `sqrt` by immrsd · Pull Request #286 · OpenZeppelin/contracts-sui

immrsd · 2026-04-01T13:26:49Z

Resolves #90

Summary by CodeRabbit

Release Notes

New Features
- Added square root operations for fixed-point number types
- Extended API documentation for fixed-point arithmetic operations and casting routes
Tests
- Added comprehensive test coverage for square root functionality

coderabbitai · 2026-04-01T13:27:06Z

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 033938cd-b7c9-4cc5-9a35-2e3cbf549807

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

Walkthrough

Added square root functionality to the fixed-point math library by introducing a shared fp_helpers module containing a sqrt_floor helper function implementing Newton's method, implementing sqrt for both SD29x9 and UD30x9 types, updating API documentation, and providing comprehensive test suites for both implementations.

Changes

Cohort / File(s)	Summary
Documentation `math/fixed_point/README.md`	Expanded API documentation to list additional arithmetic operations (`abs`, `ceil`, `div`, `floor`, `mul`, `pow`, `sqrt`) and added new casting/helper routes between types and constructor methods.
Core Implementation `math/fixed_point/sources/fp_helpers.move`, `math/fixed_point/sources/sd29x9/sd29x9_base.move`, `math/fixed_point/sources/ud30x9/ud30x9_base.move`	Introduced shared `sqrt_floor(a: u256)` helper using Newton's method for floor square root computation, implemented `sqrt(x: SD29x9)` with sign validation and magnitude scaling, and implemented `sqrt(x: UD30x9)` with raw value unwrapping and re-wrapping.
Public API Re-exports `math/fixed_point/sources/sd29x9/sd29x9.move`, `math/fixed_point/sources/ud30x9/ud30x9.move`	Added public re-exports mapping `sqrt` functions from base modules to top-level type namespaces for API consistency.
Test Coverage `math/fixed_point/tests/sd29x9_tests/sqrt_tests.move`, `math/fixed_point/tests/ud30x9_tests/sqrt_tests.move`	Added comprehensive test suites covering zero/one/perfect squares, fractional inputs, irrational truncation, floor property verification, roundtrip correctness, edge cases (max/min values), and monotonicity for both types; SD29x9 tests also verify negative input rejection.

Sequence Diagram(s)

sequenceDiagram
    participant UD30x9 as UD30x9::sqrt()
    participant SD29x9 as SD29x9::sqrt()
    participant Helper as fp_helpers::sqrt_floor()
    participant Newton as Newton Iteration
    
    rect rgba(100, 150, 255, 0.5)
    UD30x9->>Helper: sqrt_floor(raw * SCALE)
    end
    
    rect rgba(100, 200, 100, 0.5)
    SD29x9->>Helper: sqrt_floor(magnitude * SCALE)
    end
    
    rect rgba(200, 100, 100, 0.5)
    Helper->>Newton: Initial approximation + iterations
    Newton->>Newton: (xn + a/xn) >> 1
    Newton->>Helper: Refined result
    end
    
    Helper-->>UD30x9: floor(sqrt(a))
    Helper-->>SD29x9: floor(sqrt(a))
    UD30x9->>UD30x9: wrap(result)
    SD29x9->>SD29x9: wrap(result as SD29x9)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Add ud30x9 , sd29x9 , and base functions #129: Introduced the UD30x9 and SD29x9 fixed-point types that this PR extends with sqrt functionality.
Fix: SD29x9 overflow behaviour #203: Modifies the sd29x9_base module's internal structure and error handling, which directly overlaps with the sqrt implementation changes in this PR.

Suggested reviewers

0xNeshi
bidzyyys
ericnordelo

Poem

🐰 A rabbit hops through numbered fields so bright,
Where square roots bloom with Newton's guiding light,
From sqrt_floor to SD29x9,
Fixed-point math now feels so divine,
Tests and helpers dancing—oh what a sight! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The PR description only contains 'Resolves `#90`' and is missing required checklist items (Tests, Documentation, Changelog) that should be marked as complete or pending.	Complete the PR description by adding the checklist with status updates for Tests, Documentation, and Changelog items to indicate what was done.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'Fixed point `sqrt`' is concise and clearly describes the main change: implementing square root functionality for fixed-point types.
Linked Issues check	✅ Passed	The PR successfully implements square root for fixed-point math, as requested in issue `#90`. Multiple commits add sqrt helper functions, implementations for SD29x9 and UD30x9, comprehensive tests, and documentation updates.
Out of Scope Changes check	✅ Passed	All changes are directly related to implementing sqrt functionality: helper functions, implementations for both fixed-point types, tests, and README documentation. No out-of-scope changes detected.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch fixed-point-sqrt

Tip

💬 Introducing Slack Agent: The best way for teams to turn conversations into code.

Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.

Generate code and open pull requests
Plan features and break down work
Investigate incidents and troubleshoot customer tickets together
Automate recurring tasks and respond to alerts with triggers
Summarize progress and report instantly

Built for teams:

Shared memory across your entire org—no repeating context
Per-thread sandboxes to safely plan and execute work
Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get started

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

codecov · 2026-04-01T13:30:41Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.23%. Comparing base (bd055cd) to head (db5b40b).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #286      +/-   ##
==========================================
+ Coverage   90.32%   95.23%   +4.90%     
==========================================
  Files          21       21              
  Lines        1882     1910      +28     
  Branches      521      531      +10     
==========================================
+ Hits         1700     1819     +119     
+ Misses        160       69      -91     
  Partials       22       22

Flag	Coverage Δ
contracts/access	`44.33% <ø> (ø)`
math/core	`86.30% <ø> (ø)`
math/fixed_point	`49.08% <100.00%> (-15.47%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

bidzyyys

Left some comments. @immrsd resolve conflicts first.

0xNeshi

Looks good, a couple of small comments

ericnordelo

We still need to update the CHANGELOG, besides that it looks good to me.

0xNeshi

Excellent!

Missing CHANGELOG entry

bidzyyys

Looks good, my only concern is about the way you imported the openzeppelin-math library.

bidzyyys · 2026-05-11T12:23:35Z

 edition = "2024"

+[dependencies]
+openzeppelin_math = { local = "../core" }


Shouldn't it be imported with 3 sections?
1.mainnet
2. testnet
3. dev-dependencies?

This approach should copy/paste the function at deployment so there's no environment linking. What I had in mind was linking through the mvr not locally. But if we confirm with Sui (and we must before merging) that this indeed copies only the required function source code and not the whole math package we can continue with this approach.

immrsd added 6 commits April 1, 2026 15:51

Add fixed-point helper for sqrt calculation

7c5064c

Implement sqrt for SD29x9 type

c0631d6

Implement sqrt for UD30x9

45d9674

Add tests for fixed-point sqrt impl

59e4a9e

Add sqrt and remaining functions to README

f100373

Merge main

25fd007

immrsd self-assigned this Apr 1, 2026

immrsd requested review from 0xNeshi, bidzyyys and ericnordelo as code owners April 1, 2026 13:26

Format files

06f659d

0xNeshi reviewed Apr 1, 2026

View reviewed changes

Use sqrt_floor implementation from math/core package

cedb015

bidzyyys reviewed Apr 27, 2026

View reviewed changes

Comment thread math/fixed_point/sources/ud30x9/ud30x9_base.move Outdated

Comment thread math/fixed_point/sources/internal/common.move Outdated

immrsd added 6 commits April 29, 2026 14:13

Address review comments

699d6af

Merge main

63e7155

Move sqrt_floor function from fp_helpers to common module

2aa9db4

Fix tests to use assert_eq instead of deprecated expect

7e9cb7f

Remove redundant zero check in ud30x9::sqrt

b62c72c

Format files

53fd0bd

0xNeshi reviewed May 7, 2026

View reviewed changes

Comment thread math/fixed_point/tests/sd29x9_tests/sqrt_tests.move

Comment thread math/fixed_point/tests/ud30x9_tests/sqrt_tests.move Outdated

bidzyyys and others added 2 commits May 8, 2026 05:34

Merge branch 'main' into fixed-point-sqrt

bcf64d4

Address review comments

db5b40b

ericnordelo approved these changes May 11, 2026

View reviewed changes

0xNeshi previously approved these changes May 11, 2026

View reviewed changes

bidzyyys approved these changes May 11, 2026

View reviewed changes

Conversation

immrsd commented Apr 1, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Release Notes

Uh oh!

coderabbitai Bot commented Apr 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

Suggested reviewers

Poem

❌ Failed checks (1 warning)

Uh oh!

codecov Bot commented Apr 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bidzyyys left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

0xNeshi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ericnordelo left a comment

Choose a reason for hiding this comment

Uh oh!

0xNeshi left a comment

Choose a reason for hiding this comment

Uh oh!

bidzyyys left a comment

Choose a reason for hiding this comment

Uh oh!

bidzyyys May 11, 2026

Choose a reason for hiding this comment

Uh oh!

ericnordelo May 11, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

immrsd commented Apr 1, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Apr 1, 2026 •

edited

Loading

codecov Bot commented Apr 1, 2026 •

edited

Loading